-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Migrate edit product to inertia #3255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Migrate edit product to inertia #3255
Conversation
…ess` before_action in links controller.
- Extract shared permitted attributes in LinkPolicy for better maintainability. - Update routes for product editing to include separate paths for each tab (product, content, receipt, share). - Enhance controller specs to support both Inertia and JSON API requests for product updates across all tabs.
- Convert publish/unpublish to use Inertia router.post() instead of legacy AJAX saveProduct/setProductPublished utilities - Remove JSON rendering from product edit controllers (now Inertia-only) - Update save() in ProductEditLayout to return Promise for proper sequencing with publish - Add Inertia support to links_controller publish/unpublish actions with flash messages and redirects - Remove JSON API tests from controller specs (now 9 tests) - Clean up unused imports in Layout.tsx This addresses reviewer feedback about using server-side flash messages instead of client-side showAlert for success/error handling.
- Add tests for publish with Inertia request (redirect + flash notice)
- Add tests for publish error handling with Inertia (flash error)
- Add tests for unpublish with Inertia request (redirect + flash notice)
- Add basic unpublish JSON test for completeness
…ontrollers - Implemented shared examples for authorization checks in Products::Edit::ContentController, Products::Edit::ProductController, Products::Edit::ReceiptController, and Products::Edit::ShareController. - Added tests to ensure collaborators can access update actions in the respective controllers. - Enhanced existing tests to cover additional scenarios, including handling of product attributes and settings.
Resolved conflicts: - app/controllers/links_controller.rb: keep branch (no edit/update; handled by Products::Edit::*) - app/policies/link_policy.rb: add update_purchases_content? from main, keep shared_tab_permitted_attributes - spec/controllers/links_controller_spec.rb: keep branch (no GET edit / PUT update specs)
…c product props for each tab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each edit tab now gets only the product props it needs: Product tab gets full product; Content gets name, files, rich_content, variants, etc.; Receipt gets receipt/custom text fields; Share gets tags, taxonomy_id, display_product_reviews, is_adult, custom_domain. Layout still gets shared top-level props via layout_props (no product). This avoids sending pricing/refund/share data to tabs that don’t use it.
| resource :product, only: [:edit, :update], controller: "edit/product" | ||
| resource :content, only: [:edit, :update], controller: "edit/content" | ||
| resource :receipt, only: [:edit, :update], controller: "edit/receipt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped path: "product" from the product resource; the default path for a singular :product resource is already "product", so behavior is unchanged and the config is simpler.
| # TODO: :product_edit_react cleanup | ||
| if option[:price_difference_cents].present? | ||
| option[:price] = option[:price_difference_cents] | ||
| option[:price] = option[:price_difference_cents].to_i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
price_difference_cents can be nil when not sent (e.g. non-coffee variant) or blank, and that .to_i is used to coerce string params from forms. The existing .present? guard and .to_i logic are unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added specs to ensure each tab presenter returns layout props plus only that tab’s product keys (e.g. Content/Receipt/Share do not include price_cents, refund_policy). BasePresenter specs cover layout_props (no product) and product_minimal_props (name, is_published, files, native_type).
sahitya-chandra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self review
|
@EmCousin sir, I have incorporated your review comments in #3202. Could you take another look when you have time? Thanks. Cc @Pradumn27 |
| def fetch_product | ||
| product_id = params[:product_id] || params[:id] | ||
| @product = Link.fetch_leniently(product_id, user: current_seller) || Link.fetch_leniently(product_id) | ||
| raise(ActiveRecord::RecordNotFound) if @product.nil? || @product.archived? || @product.deleted_at.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to be able to edit archived products? What is the reason you added this constraint here? Right now it breaks the behaviour when I want to click on Edit in the Archived products list
| get "/s3_utility/generate_multipart_signature", to: "s3_utility#generate_multipart_signature" | ||
|
|
||
| constraints GumroadDomainConstraint do | ||
| resources :products, only: [], param: :id do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already the case by default.
| resources :products, only: [], param: :id do | |
| resources :products, only: [] do |
| module Products | ||
| module Edit | ||
| class BasePresenter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not nest modules/classes in this code base, the general style is to declare those inline (class Products::Edit::BasePresenter)
| end | ||
|
|
||
| def fetch_product | ||
| product_id = params[:product_id] || params[:id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, there is no case where the product_id will be present in params[:id] when hitting the child controllers' endpoints, it will always be in params[:product_id]
| end | ||
|
|
||
| # Full product hash for the Product tab only. | ||
| def product_tab_product_props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be defined in ProductTabPresenter
| module Edit | ||
| class ContentController < BaseController | ||
| def edit | ||
| render inertia: "Products/Edit/Content", props: Products::Edit::ContentTabPresenter.new(product: @product, pundit_user:).props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref. comment above, the template should be Products/Content/Edit
| ProfileSection, | ||
| ShippingCountry, | ||
| } from "$app/components/ProductEdit/state"; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still a lot of tight coupling in this layout, there should be conditionals in there do determine what to do depending on which pages we are on. Each page should instead have their own edit props, their own form object and their own update function
| const pathname = window.location.pathname; | ||
| let updateUrl: string; | ||
| if (pathname.includes("/content/edit")) updateUrl = Routes.product_content_path(uniquePermalink); | ||
| else if (pathname.includes("/receipt/edit")) updateUrl = Routes.product_receipt_path(uniquePermalink); | ||
| else if (pathname.includes("/share/edit")) updateUrl = Routes.product_share_path(uniquePermalink); | ||
| else updateUrl = Routes.product_product_path(uniquePermalink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be determining the endpoint url in the layout based on conditionals, instead each edit page should have their own update function
| const setPublished = async (published: boolean) => { | ||
| try { | ||
| setIsPublishing(true); | ||
| await saveProduct(uniquePermalink, id, product, currencyType); | ||
| await setProductPublished(uniquePermalink, published); | ||
| updateProduct({ is_published: published }); | ||
| showAlert(published ? "Published!" : "Unpublished!", "success"); | ||
| if (tab === "share") { | ||
| if (product.native_type === "coffee") navigate.current(rootPath); | ||
| else navigate.current(`${rootPath}/content`); | ||
| } else if (published) { | ||
| navigate.current(`${rootPath}/share`); | ||
| setIsPublishing(true); | ||
| if (published) { | ||
| try { | ||
| await save(); | ||
| } catch { | ||
| setIsPublishing(false); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (published) { | ||
| router.post(Routes.publish_link_path(uniquePermalink), {}, { | ||
| preserveScroll: true, | ||
| onSuccess: () => updateProduct({ is_published: true }), | ||
| onFinish: () => setIsPublishing(false), | ||
| }); | ||
| } else { | ||
| // Unpublish: use current tab's update URL with unpublish (like bundles) | ||
| if (tab === "product" || tab === "content" || tab === "share") { | ||
| const unpublishUrl = | ||
| tab === "product" | ||
| ? Routes.product_product_path(uniquePermalink) | ||
| : tab === "content" | ||
| ? Routes.product_content_path(uniquePermalink) | ||
| : Routes.product_share_path(uniquePermalink); | ||
| router.visit(unpublishUrl, { | ||
| method: "patch", | ||
| data: { unpublish: true }, | ||
| preserveScroll: true, | ||
| onSuccess: () => updateProduct({ is_published: false }), | ||
| onFinish: () => setIsPublishing(false), | ||
| }); | ||
| } else { | ||
| router.post(Routes.unpublish_link_path(uniquePermalink), {}, { | ||
| preserveScroll: true, | ||
| onSuccess: () => updateProduct({ is_published: false }), | ||
| onFinish: () => setIsPublishing(false), | ||
| }); | ||
| } | ||
| } catch (e) { | ||
| assertResponseError(e); | ||
| showAlert(e.message, "error", { html: true }); | ||
| } | ||
| setIsPublishing(false); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not make sense. Publishing/Unpublishing a product is now part of the update function by passing either publish or unpublish to the request's body, then we told the backend what to do based on the presence or absence of those values. You should not be needing the publish_link_path nor the unpublish_link_path at all. Again, look how it's done for updating a bundle.
| def unpublish | ||
| authorize @product | ||
|
|
||
| @product.unpublish! | ||
| render json: { success: true } | ||
|
|
||
| if request.inertia? | ||
| flash[:notice] = "Unpublished!" | ||
| redirect_to product_edit_redirect_url | ||
| else | ||
| render json: { success: true } | ||
| end | ||
| end | ||
|
|
||
| def publish | ||
| authorize @product | ||
|
|
||
| if @product.user.email.blank? | ||
| return render json: { success: false, error_message: "<span>To publish a product, we need you to have an email. <a href=\"#{settings_main_url}\">Set an email</a> to continue.</span>" } | ||
| error_message = "<span>To publish a product, we need you to have an email. <a href=\"#{settings_main_url}\">Set an email</a> to continue.</span>" | ||
| if request.inertia? | ||
| flash[:alert] = error_message.html_safe | ||
| return redirect_back fallback_location: edit_product_product_path(@product.unique_permalink) | ||
| else | ||
| return render json: { success: false, error_message: } | ||
| end | ||
| end | ||
|
|
||
| begin | ||
| @product.publish! | ||
| rescue Link::LinkInvalid, ActiveRecord::RecordInvalid | ||
| return render json: { success: false, error_message: @product.errors.full_messages[0] } | ||
| error_message = @product.errors.full_messages[0] | ||
| if request.inertia? | ||
| flash[:alert] = error_message | ||
| return redirect_back fallback_location: edit_product_product_path(@product.unique_permalink) | ||
| else | ||
| return render json: { success: false, error_message: } | ||
| end | ||
| rescue => e | ||
| Bugsnag.notify(e) | ||
| return render json: { success: false, error_message: "Something broke. We're looking into what happened. Sorry about this!" } | ||
| error_message = "Something broke. We're looking into what happened. Sorry about this!" | ||
| if request.inertia? | ||
| flash[:alert] = error_message | ||
| return redirect_back fallback_location: edit_product_product_path(@product.unique_permalink) | ||
| else | ||
| return render json: { success: false, error_message: } | ||
| end | ||
| end | ||
|
|
||
| render json: { success: true } | ||
| if request.inertia? | ||
| flash[:notice] = "Published!" | ||
| redirect_to edit_product_share_path(@product.unique_permalink) | ||
| else | ||
| render json: { success: true } | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These endpoints should not be used anymore since publishing/unpublishing is part of the update action in each products edit page
Issue: #3030
Closes #3030
Parent issue: #3028
previous pr: #3096 and #3166 and #3202
Description
Problem
The Edit Product page (
/products/:id/edit) used a single React on Rails server-rendered flow with one monolithicLinksController#editandLinksController#update. All tabs (Product, Content, Receipt, Share) were handled viaparams[:tab], causing full page reloads when switching tabs and sending unnecessary data for each tab.Solution
Migrated the Edit Product page to Inertia with per-tab server endpoints, matching the pattern used in merged PRs (Bundles #3173).
Approach:
Dedicated controller per tab – Each tab has its own GET (edit) and PATCH/PUT (update) endpoint with resource-style URLs:
/products/:id/product/edit→Products::Edit::ProductController/products/:id/content/edit→Products::Edit::ContentController/products/:id/receipt/edit→Products::Edit::ReceiptController/products/:id/share/edit→Products::Edit::ShareController/products/:id/edit→/products/:id/product/edit).Decoupled props per tab – Each edit page receives only the props it needs.
Products::Edit::BasePresenterprovides:layout_props– shared top-level props (id, unique_permalink, seller, currency_type, taxonomies, etc.) with no product.product_tab_product_props(full product for Product tab),content_tab_product_props(name, files, rich_content, variants, etc.),receipt_tab_product_props(name, custom_receipt_text, custom_view_content_button_text, etc.),share_tab_product_props(name, tags, taxonomy_id, display_product_reviews, is_adult, custom_domain, etc.). Each tab presenter mergeslayout_propswith its tab’s product hash so Content/Receipt/Share do not receive pricing, refund, or other unrelated fields.Inertia pages – One page component per tab under
app/javascript/pages/Products/Edit/(Product.tsx, Content.tsx, Receipt.tsx, Share.tsx), with a sharedProductEditLayout.tsxusing Inertia’suseForm()for save.Dual Inertia + JSON – Product edit controllers support both:
form.patch()from the "Save changes" button → redirect + flash.{ success: true }.Publish/unpublish in
LinksControllersupport both: Inertia — "Save and continue" / "Unpublish" callsave()(form.patch) thenrouter.post(), with server-side flash ("Published!" / "Unpublished!"); JSON — for other callers.Cleanup – Removed old React on Rails entry:
packs/product_edit.ts,views/links/edit.html.erb,ProductEditPage.tsxserver component, and the monolithicedit/updateactions fromLinksController.Key files:
app/controllers/products/edit/base_controller.rb– Base for all tab controllers,layout "inertia".app/controllers/products/edit/*_controller.rb– Tab-specific edit/update withrequest.inertia?for dual response.app/presenters/products/edit/base_presenter.rb–layout_propsand tab-specific product prop methods; tab presenters compose props per tab.app/presenters/products/edit/*_tab_presenter.rb– Each returnslayout_props.merge(product: <tab>_tab_product_props).app/javascript/layouts/ProductEditLayout.tsx– Inertia layout withuseForm(),form.patch()for save.app/javascript/pages/Products/Edit/*.tsx– Per-tab Inertia pages.app/javascript/components/ProductEdit/Layout.tsx– Tab chrome, Save/Publish/Unpublish; uses InertiaLink/router.visitandrouter.postfor publish/unpublish.app/policies/link_policy.rb– Permitted attributes split into*_tab_permitted_attributesper tab.Other changes:
config/initializers/react_on_rails.rb–controller.params = ActionController::Parameters.new(params)so React on Rails SSR (e.g. checkout) works with strong parameters. Pre-existing bug fix, not specific to this migration.app/javascript/packs/inertia.js– CSRF token set onrequestDefaults.headersfor non-Inertia AJAX (image uploads, tag suggestions) from Product Edit components.Before / After
Screencast.from.2026-01-28.19-04-58.webm
Screencast.from.2026-01-30.22-02-04.webm
Implementation notes
resources :productswith per-tabresourcefor edit/update (edit_product_product_path, etc.). No explicitpath: "product"; default path is used. Backward-compatibility redirects for legacy URLs.notice:/alert:in the redirect and usestatus: :see_other(303). No separateflashassignment before redirect.{ unpublish: true }and to avoid membership tier validation. Share tab runs update in transaction then unpublish; Receipt tab usesLinksController#unpublishPOST.rescueat the end of each update action (nobegin/rescue). All error redirects usestatus: :see_other.Product::VariantsUpdaterService:price_difference_centscan be nil when not sent (e.g. non-coffee variant) or blank;.to_icoerces string params from forms. Comment added in the service.Test results
spec/controllers/products/edit/(Product, Content, Receipt, Share; GET edit + PATCH update, authorize/collaborator, tab-specific behavior).spec/presenters/products/edit/(BasePresenter layout_props/product_minimal_props; ProductTabPresenter, ContentTabPresenter, ReceiptTabPresenter, ShareTabPresenter each return layout props + tab-specific product props only).spec/controllers/links_controller_spec.rb(POST publish at line 51, POST unpublish at line 162). Total: 33 controller + 6 presenter + 15 publish/unpublish = 54 examples for this migration.Run:
bundle exec rspec spec/controllers/products/edit/ spec/presenters/products/edit/ spec/controllers/links_controller_spec.rb:51 spec/controllers/links_controller_spec.rb:162 --format documentationChecklist
AI Disclosure
used cursor with opus-4.5 to plan the solution and refactoring code
AI was also used to:
LinkPolicyto split permitted attributes.saveProductfrontend logic to use dynamic Inertia endpoints.